Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Quantile aggregation, DDSketch aggregator; add Exact timestamps #1412

Merged
merged 21 commits into from
Jan 12, 2021

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 18, 2020

This is a cleanup of an area in the code that is not well exercised.

  • Renames aggregator/array to aggregator/exact
  • Add missing exact timestamps to the exact aggregator
  • Remove Quantile and Distribution export aggregation interfaces
  • Remove DDSketch aggregator; we expect this will return after more histogram types are available.

Before:

goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
BenchmarkInt64ArrayAdd-8           	 3018822	       369 ns/op	     301 B/op	       2 allocs/op
BenchmarkInt64ArrayHandleAdd-8     	27656120	        55.6 ns/op	      46 B/op	       0 allocs/op
BenchmarkFloat64ArrayAdd-8         	 3263607	       423 ns/op	     298 B/op	       2 allocs/op
BenchmarkFloat64ArrayHandleAdd-8   	27069272	        56.4 ns/op	      47 B/op	       0 allocs/op

After:

BenchmarkInt64ExactAdd-8           	 1657836	       711 ns/op	     419 B/op	       2 allocs/op
BenchmarkInt64ExactHandleAdd-8     	 6865136	       208 ns/op	     188 B/op	       0 allocs/op
BenchmarkFloat64ExactAdd-8         	 2269024	       566 ns/op	     442 B/op	       2 allocs/op
BenchmarkFloat64ExactHandleAdd-8   	 5736513	       178 ns/op	     180 B/op	       0 allocs/op

Although this updates the exact aggregator, that code is still not used within this repository. A future PR may add support to the OTLP exporter to export raw point values and timestamps.

Closes #1417

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #1412 (93feac1) into master (9c94941) will increase coverage by 0.0%.
The diff coverage is 88.1%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #1412    +/-   ##
=======================================
  Coverage    78.8%   78.8%            
=======================================
  Files         131     130     -1     
  Lines        6753    6642   -111     
=======================================
- Hits         5326    5239    -87     
+ Misses       1167    1148    -19     
+ Partials      260     255     -5     
Impacted Files Coverage Δ
exporters/metric/prometheus/prometheus.go 64.5% <0.0%> (+6.9%) ⬆️
exporters/stdout/config.go 70.0% <ø> (-6.0%) ⬇️
exporters/stdout/exporter.go 18.5% <0.0%> (-7.5%) ⬇️
exporters/stdout/metric.go 76.3% <ø> (-0.2%) ⬇️
sdk/export/metric/aggregation/aggregation.go 100.0% <ø> (ø)
sdk/metric/aggregator/aggregatortest/test.go 84.8% <ø> (-0.2%) ⬇️
sdk/metric/aggregator/minmaxsumcount/mmsc.go 96.0% <ø> (ø)
exporters/otlp/internal/transform/metric.go 80.5% <75.0%> (ø)
sdk/metric/processor/processortest/test.go 89.7% <93.3%> (+0.1%) ⬆️
sdk/metric/aggregator/exact/exact.go 94.4% <94.4%> (ø)
... and 3 more

@jmacd
Copy link
Contributor Author

jmacd commented Dec 21, 2020

@open-telemetry/specs-metrics-approvers As I am trying to bring the OTel-Go metrics SDK into line with "Everything we will specify", this Quantile aggregation is something I'd like to remove. I was on the fence about adding timestamps and renaming the Array->Exact aggregator, but I think we want to keep an Exact aggregator that captures timestamps, so this is a small change in the forward direction.

(Note: I'm hoping to relieve the @open-telemetry/go-approvers of this responsibility.)

sdk/metric/aggregator/exact/exact.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/exact/exact_test.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/exact/exact_test.go Outdated Show resolved Hide resolved
@krnowak
Copy link
Member

krnowak commented Dec 21, 2020

The PR looks good, just found some nitpicks.

sdk/export/metric/aggregation/aggregation.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/exact/exact.go Outdated Show resolved Hide resolved
sdk/metric/aggregator/exact/exact_test.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Jan 5, 2021

@bogdandrutu Please review.

exporters/otlp/internal/transform/metric.go Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would encourage to split this PR:

  1. Changes to array/exact aggregator
  2. Remove Quantile and Distribution export aggregation interfaces
  3. Remove DDSketch aggregator; we expect this will return after more histogram types are available.

@jmacd
Copy link
Contributor Author

jmacd commented Jan 7, 2021

@bogdandrutu Yes the three parts of this PR are logically independent and could be split in sequence. I think it's way more trouble than it's worth though.

@bogdandrutu
Copy link
Member

@jmacd I am not the maintainer, so my comment is just a suggestion. If I was the maintainer I would have pushed for this :)

@MrAlias
Copy link
Contributor

MrAlias commented Jan 8, 2021

We talked about splitting this up in the SIG meeting yesterday and came to the decision that given the existing approvals we should progress as is. Though future PRs should follow the guidance you laid out @bogdandrutu.

@jmacd I think this can merge once the this is addressed.

@MrAlias MrAlias merged commit 49f699d into open-telemetry:master Jan 12, 2021
@jmacd jmacd deleted the jmacd/remove_quantile branch February 4, 2021 18:36
@rmrf
Copy link

rmrf commented Jul 5, 2021

Where I can find the reason to remove quantile?

@rmrf
Copy link

rmrf commented Jul 5, 2021

Where I can find the reason to remove quantile?

I found out the reason: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#summary-legacy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider moving github.com/DataDog/sketches-go to 1.0.0
9 participants